Fix and refactor UnderlineNav to resolve CLS issues and improve performance/code quality#7506
Fix and refactor UnderlineNav to resolve CLS issues and improve performance/code quality#7506iansan5653 wants to merge 71 commits intomainfrom
UnderlineNav to resolve CLS issues and improve performance/code quality#7506Conversation
…e.module.css Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…e.tsx Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ate to detect overflow
…er themselves for the menu
🦋 Changeset detectedLatest commit: 0634340 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
|
BTW, I think the flex inspector in Chrome's devtools does a really nice job of showing what's going on here / where the tabs are going: Screen.Recording.2026-02-06.at.5.00.33.PM.mov |
a787dd6 to
184ea86
Compare
…mer/react into underline-nav-full-css-spike
|
@copilot add a changeset for this pr to the |
|
@iansan5653 I've opened a new pull request, #7622, to work on those changes. Once the pull request is ready, I'll request review from you. |
…formance/code quality (#7622) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: iansan5653 <2294248+iansan5653@users.noreply.github.com>
hectahertz
left a comment
There was a problem hiding this comment.
Really nice refactor! The shift from JS width measurements to CSS wrap-based overflow detection is a big win for CLS, and the createDescendantRegistry utility is clean and well-tested. Great code removal too.
Left some comments, nothing major.
| }) | ||
| // This is the case where the viewport is too narrow to show any list item with the more menu. In this case, we only show the dropdown | ||
| const onlyMenuVisible = responsiveProps.items.length === 0 | ||
| const isOverflowing = menuItems.length > 0 |
There was a problem hiding this comment.
This setState during render works (React allows it for the component's own state), but it will trigger an immediate re-render on first overflow detection. Totally fine in practice, just flagging since useEffect is the more common pattern.
There was a problem hiding this comment.
Yep, React will abandon this render and start a new one. It's uncommon but probably should be more common - React recommends it over using an effect
| /* Progressive enhancement: Detect overflow using scroll-based animations. | ||
| The idiomatic way would be a scroll-state container query but browser support | ||
| is slightly better for animations. */ | ||
| animation: detect-overflow linear; |
There was a problem hiding this comment.
Nice progressive enhancement! animation-timeline: scroll() isn't supported in Safari/Firefox yet, so it's good there's a JS fallback via data-has-overflow.
One thought: between first paint and the first IO callback, overflowing items will be clipped but the "More" button won't be visible yet. Is that flash acceptable, or should we show the button by default and hide it once we confirm nothing overflows?
There was a problem hiding this comment.
This is a great question, I was wondering the same thing. The challenge is that we have to choose - on small screens do we delay showing the button, or on large screens do we show and then hide it when there's no overflow?
I think, given the choice, it's typically preferable to insert new UI after SSR vs hiding UI that you initially showed. Showing and then hiding something feels more flickery than showing it after a short delay. I also think large screens are probably more common.
To be clear, this flash would only happen in browsers that don't support scroll animation.
We could try and be more clever here by showing it by default on small screens based on some arbitrary breakpoint, trying to predict when there's most likely going to be an overflow. But I'm not sure it's worth the complexity and it could never be 100% reliable since we can't know what width would trigger the overflow until after we calculate the overflowed items.
This refact/bugfix PR reworks
UnderlineNav's internal logic to rely mostly on CSS for calculating overflow, eliminating layout shift issues. In addition, it removes the use of theReact.ChildrenAPI in favor of context-based solutions, and replaces the custom overflow menu with anActionMenu.See https://github.com/github/primer/issues/6291#issuecomment-3861950186 for context.
The CSS overflow logic works like this:
overflow: hidden, creating the illusion that they disappearedscroll-statecontainer queries to detect overflow using pure CSSIntersectionObserverto update itself with the parent when it appears/disappears. They determine whether they are overflowed by checkingref.current.offsetHeight(which will be greater than zero for an item that has wrapped). If they are overflowed, they setaria-hidden/tabIndex="-1"and notify the parent via the registry.The benefits are pretty significant:
ChildrenAPIs entirelyChildrenAPIs, we no longer need direct children, so consumers can wrap their underline items in fragments and even wrapper componentsHowever, there are some caveats / user-facing changes:
Changelog
New
Changed
Removed
Rollout strategy
Testing & Reviewing
Merge checklist